Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Export plotly-express as a dashboard plugin #329

Merged
merged 3 commits into from
Mar 5, 2024
Merged

feat: Export plotly-express as a dashboard plugin #329

merged 3 commits into from
Mar 5, 2024

Conversation

vbabich
Copy link
Contributor

@vbabich vbabich commented Feb 28, 2024 •
edited
Loading

Tested with DHE V+ (1.20231218.176) Core 0.32.1
Closes #308

BREAKING CHANGE:

  • widget type in the PanelEvent.OPEN event arguments has changed from VariableDefinition to VariableDescriptor

@vbabich vbabich requested a review from mofojed February 28, 2024 17:26
@vbabich vbabich self-assigned this Feb 28, 2024
@vbabich vbabich added this to the February 2024 milestone Feb 28, 2024
@mattrunyon
Copy link
Collaborator

I think this will break plotly-express in DHC w/ dh.ui. We currently check if the plugin is the legacy format first when loading, so the legacy definition (named DashboardPlugin export) will override the preferred definition (default export of the plugin object)

@vbabich vbabich force-pushed the plotly-express-dashboard-plugin branch from cb4dc26 to e8ed513 Compare February 29, 2024 21:26
@vbabich vbabich marked this pull request as ready for review February 29, 2024 21:27
@vbabich
Copy link
Contributor Author

vbabich commented Feb 29, 2024

I added a named export for the legacy DashboardPlugin next to the default WidgetPlugin.
Updated DHE to prioritize legacy plugins until we implement WidgetLoaderPlugin in Enterprise.

@vbabich vbabich requested a review from mofojed March 4, 2024 19:30
mofojed
mofojed approved these changes Mar 5, 2024
View reviewed changes
plugins/plotly-express/src/js/src/DashboardPlugin.tsx
): JSX.Element | null {
const { id, layout, registerComponent } = props;

const handlePanelOpen = useCallback(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note, we have a useDashboardPanel hook that should do what we're doing here, but it would require other changes as well (since it doesn't use metadata or fetch at all). No need to worry about it here.

mattrunyon
mattrunyon approved these changes Mar 5, 2024
View reviewed changes
Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll actually want DHE to register both plugins. Otherwise plotly-express won't work in dh.ui in DHE

@vbabich
Copy link
Contributor Author

vbabich commented Mar 5, 2024

Why? What plugin type is needed for dh.ui in DHE?

@mattrunyon
Copy link
Collaborator

dh.ui reads the plugin registry and looks for WidgetPlugins to render widgets within panels. So if you have a dh.ui panel with a table and plotly-express plot, it needs both as WidgetPlugins to render properly. Just the DashboardPlugin won't do anything since that's listening at the dashboard level for a plotly-express widget to be opened as a panel.

@vbabich
Copy link
Contributor Author

vbabich commented Mar 5, 2024

But it should work with just the WidgetPlugin, right? The plan is to phase out DashboardPlugin once we add support for WidgetPlugin in DHE.

@mattrunyon
Copy link
Collaborator

Yes once we support WidgetPlugins auto opening like in DHC, we'll only need the WidgetPlugin. I think we'll need both in the interim in DHE so that outside of dh.ui the DashboardPlugin will open the panel, but in dh.ui, the WidgetPlugin is needed so dh.ui knows how to render the widget

@vbabich vbabich merged commit 6212bd5 into main Mar 5, 2024
13 checks passed
@vbabich vbabich deleted the plotly-express-dashboard-plugin branch March 5, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattrunyon mattrunyon mattrunyon approved these changes

@mofojed mofojed mofojed approved these changes

Assignees

@vbabich vbabich

Labels
None yet
Projects
None yet
Milestone
February 2024
Development

Successfully merging this pull request may close these issues.

DH-16500: Release plotly-express js plugin version that can be loaded as a dashboard plugin
3 participants
@vbabich @mattrunyon @mofojed